Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consider admin defaults when creating shares #43024

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Consider admin defaults when creating shares #43024

merged 3 commits into from
Feb 1, 2024

Conversation

nfebe
Copy link
Contributor

@nfebe nfebe commented Jan 22, 2024

The current share logic always uses the default BUNDLED_PERMISSIONS.ALL which includes everything. This commit updates share creation logic to use defaultPermissions if set by admin for the creation of new shares.

Resolves: #42835

@nfebe nfebe force-pushed the 42835-use-default-perms-4new-shares branch 2 times, most recently from f09aea3 to e8ff76f Compare January 22, 2024 17:39
nfebe
nfebe commented Jan 23, 2024
View reviewed changes
Copy link
Contributor Author

@nfebe nfebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default permissions might not always be same as "Allow editing" (or basically all permissions)

Screenshot from 2024-01-23 12-27-32

apps/files_sharing/src/views/SharingDetailsTab.vue Outdated Show resolved Hide resolved
@nfebe
Copy link
Contributor Author

nfebe commented Jan 25, 2024

Something else that has come up while working on this in relation to maintaining "Allow editing" and "Default permissions"

  • It turns out when no permissions are set on the admin, the default permission is "1" which is ready only. So changing "Allow editing" to "Default permissions" which has the probability to be 1, most of the time would make little sense with read only just at the top, that is, "Read only" (first option), second option (Default permissions, still read only)

Screenshot from 2024-01-25 11-29-58

So, the brainstorming is not over it seems, the second path, is simply to add "Default permissions" as a new option IF it is different from READ only (1) as well as different from "Allow editing" (31)

@nfebe
Copy link
Contributor Author

nfebe commented Jan 25, 2024

cc: @jancborchardt

@jancborchardt
Copy link
Member

As far as I can see, the issue reported at #42835 should be fixed via code and does not need a new entry in this list of presets.
These are merely default share permissions set by the admin and that’s that.

  • If the permissions set by the admin are the same as "View only", preselect that
  • If the permissions set by the admin are the same as "Allow editing", preselect that
  • If the permissions set by the admin are anything else, preselect "Custom permissions" with the relevant subline

But we should not replace an entry of those with "Default permissions" or anything like that.

@nfebe
Copy link
Contributor Author

nfebe commented Jan 25, 2024

As far as I can see, the issue reported at #42835 should be fixed via code and does not need a new entry in this list of presets.
These are merely default share permissions set by the admin and that’s that.

Sure, it does not necessarily need a new entry in the list of presets. But definitely needs making changes to how the presets work to avoid confusion. Adding another option or updating language are just other solutions paths suggested.

So the new implication is that "Allow editing" is not always the selected/default option as is the case now. It would be dynamically selected.

The only difference between this and my last suggestion is that no new entry would be added. To mitigate the issue of clearly understanding what default permissions are, I think it would be nice to have a way for the user to know that whatever permission was pres-elected for them is the default. This can be done by simply adding something like Default in parenthesis. (Default)

@nfebe nfebe force-pushed the 42835-use-default-perms-4new-shares branch 2 times, most recently from 8e3a248 to db03ac1 Compare January 30, 2024 23:00
@nfebe nfebe marked this pull request as ready for review January 30, 2024 23:02
@nfebe nfebe requested review from susnux, Pytal and skjnldsv January 30, 2024 23:03
@nfebe nfebe force-pushed the 42835-use-default-perms-4new-shares branch 3 times, most recently from 4a4c59c to 388fd55 Compare January 31, 2024 20:08
@nfebe nfebe requested a review from skjnldsv January 31, 2024 20:08
@nfebe nfebe force-pushed the 42835-use-default-perms-4new-shares branch 2 times, most recently from eed249c to 0662682 Compare January 31, 2024 20:34
@nfebe nfebe force-pushed the 42835-use-default-perms-4new-shares branch from 0662682 to cd58945 Compare February 1, 2024 14:05
@nfebe
Copy link
Contributor Author

nfebe commented Feb 1, 2024

/compile /

@susnux
Copy link
Contributor

susnux commented Feb 1, 2024

/compile

CI is a bit slow at the moment due to 28.0.2 release I guess, you could short cut by compiling locally and submit the assets.

@nfebe nfebe enabled auto-merge February 1, 2024 14:36
nfebe added 2 commits February 1, 2024 17:53
@nfebe
Consider admin defaults when creating shares
d9a2a78 
The current share logic always uses the default `BUNDLED_PERMISSIONS.ALL`
which includes everything.

This commit updates share creation logic to use `defaultPermissions` if set
by admin for the creation of new shares.

Signed-off-by: fenn-cs <[email protected]>
@nfebe
Use @nextcloud/capabilities to obtain share api defaults
d1ec85d 
- Remove redundant initial state added
- Call `getCapabilities()` in share config file.

Signed-off-by: fenn-cs <[email protected]>
@nfebe nfebe force-pushed the 42835-use-default-perms-4new-shares branch from bde1408 to 9cff27b Compare February 1, 2024 16:53
@nfebe
Copy link
Contributor Author

nfebe commented Feb 1, 2024

/compile amend /

@nfebe @nextcloud-command
chore: compile assets
f944d26 
Signed-off-by: fenn-cs <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
@nextcloud-command nextcloud-command force-pushed the 42835-use-default-perms-4new-shares branch from 9cff27b to f944d26 Compare February 1, 2024 18:25
@nfebe nfebe disabled auto-merge February 1, 2024 23:42
@nfebe nfebe merged commit c5c4840 into master Feb 1, 2024
107 of 134 checks passed
@nfebe nfebe deleted the 42835-use-default-perms-4new-shares branch February 1, 2024 23:43
@nfebe
Copy link
Contributor Author

nfebe commented Feb 1, 2024

PHP unit failures unrelated.

@nfebe
Copy link
Contributor Author

nfebe commented Feb 1, 2024

/backport to stable28

@nfebe
Copy link
Contributor Author

nfebe commented Feb 1, 2024

/backport to stable27

@backportbot backportbot bot mentioned this pull request Feb 1, 2024
Merged
2 tasks
@backportbot backportbot bot mentioned this pull request Feb 1, 2024
Merged
2 tasks
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JSoko JSoko JSoko left review comments

@jancborchardt jancborchardt jancborchardt left review comments

@emoral435 emoral435 emoral435 approved these changes

@artonge artonge artonge approved these changes

@susnux susnux susnux approved these changes

@Altahrim Altahrim Awaiting requested review from Altahrim

@sorbaugh sorbaugh Awaiting requested review from sorbaugh

@Pytal Pytal Awaiting requested review from Pytal

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
27-feedback 28-feedback feature: sharing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Creating new shares doesn't use default admin sharing permissions
8 participants
@nfebe @jancborchardt @susnux @JSoko @artonge @skjnldsv @sorbaugh @emoral435